Skip to content

Conversation

BVMiko
Copy link
Contributor

@BVMiko BVMiko commented Aug 23, 2021

Issue #, if available: #646

Description of changes:

After removing path prefixes, allow an empty path to return "/" so that it will match a normal routing rule.

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 23, 2021
@michaelbrewer
Copy link
Contributor

@BVMiko - Nice catch!

Could you add a test case? Something like:

def test_api_gateway_request_path_equals_strip_prefix():
    # GIVEN a strip_prefix matches the request path
    app = ApiGatewayResolver(strip_prefixes=["/foo"])
    event = {"httpMethod": "GET", "path": "/foo"}

    @app.get("/")
    def base():
        return {}

    # WHEN calling the event handler
    # WITH a route "/"
    result = app(event, {})

    # THEN process event correctly
    assert result["statusCode"] == 200
    assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON

Regarding for the fix, i guess it could also default to "" over "/", but i would defer that to @heitorlessa

@michaelbrewer
Copy link
Contributor

@BVMiko - FYI, Heitor will be about for 5 weeks. How critical would a fix for this be?

@BVMiko
Copy link
Contributor Author

BVMiko commented Aug 23, 2021

For now I have a workaround in place which can remain until this issue is resolved:

app = ApiGatewayResolver(strip_prefixes=["/foo"])

@app.get("/")
def base():
    return {}

def lambda_handler(event, context):
    if event['path'] == '/foo':
        event['path'] = '/foo/'
    return app.resolve(event, context)

@heitorlessa heitorlessa added the bug Something isn't working label Aug 23, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #647 (a8857e0) into develop (c8cf3ba) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #647      +/-   ##
===========================================
- Coverage    99.97%   99.93%   -0.05%     
===========================================
  Files          116      116              
  Lines         4846     4821      -25     
  Branches       265      266       +1     
===========================================
- Hits          4845     4818      -27     
- Misses           0        1       +1     
- Partials         1        2       +1     
Impacted Files Coverage Δ
aws_lambda_powertools/event_handler/api_gateway.py 99.00% <0.00%> (-1.00%) ⬇️
aws_lambda_powertools/logging/logger.py 100.00% <0.00%> (ø)
aws_lambda_powertools/tracing/tracer.py 100.00% <0.00%> (ø)
aws_lambda_powertools/metrics/metrics.py 100.00% <0.00%> (ø)
aws_lambda_powertools/logging/formatter.py 100.00% <0.00%> (ø)
aws_lambda_powertools/utilities/batch/base.py 100.00% <0.00%> (ø)
aws_lambda_powertools/shared/jmespath_utils.py 100.00% <0.00%> (ø)
...ws_lambda_powertools/middleware_factory/factory.py 100.00% <0.00%> (ø)
...s_lambda_powertools/utilities/parser/models/sns.py 100.00% <0.00%> (ø)
..._lambda_powertools/utilities/feature_flags/base.py 100.00% <0.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8cf3ba...a8857e0. Read the comment docs.

@michaelbrewer
Copy link
Contributor

@BVMiko - Nice catch!

Could you add a test case? Something like:

def test_api_gateway_request_path_equals_strip_prefix():
    # GIVEN a strip_prefix matches the request path
    app = ApiGatewayResolver(strip_prefixes=["/foo"])
    event = {"httpMethod": "GET", "path": "/foo"}

    @app.get("/")
    def base():
        return {}

    # WHEN calling the event handler
    # WITH a route "/"
    result = app(event, {})

    # THEN process event correctly
    assert result["statusCode"] == 200
    assert result["headers"]["Content-Type"] == content_types.APPLICATION_JSON

Regarding for the fix, i guess it could also default to "" over "/", but i would defer that to @heitorlessa

@BVMiko - would you be able to include a test like this into the api gateway handler test suite?

@pull-request-size pull-request-size bot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 31, 2021
@boring-cyborg boring-cyborg bot added the tests label Aug 31, 2021
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 31, 2021
@BVMiko
Copy link
Contributor Author

BVMiko commented Aug 31, 2021

@michaelbrewer: The test has been added

@boring-cyborg boring-cyborg bot added the documentation Improvements or additions to documentation label Aug 31, 2021
Copy link
Contributor

@michaelbrewer michaelbrewer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks @BVMiko

@to-mc to-mc merged commit 6dee33a into aws-powertools:develop Sep 2, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 2, 2021

Awesome work, congrats on your first merged pull request and thank you for helping improve everyone's experience!

heitorlessa added a commit to heitorlessa/aws-lambda-powertools-python that referenced this pull request Sep 28, 2021
…tools-python into develop

* 'develop' of https://github.com/awslabs/aws-lambda-powertools-python:
  docs(event-handler): document catch-all routes (aws-powertools#705)
  chore: add python 3.9 support
  docs: add team behind it and email
  ISSUE-693: Use ExpressionAttributeNames in _put_record (aws-powertools#697)
  feat(validator): include missing data elements from a validation error (aws-powertools#686)
  chore(deps-dev): bump mkdocs-material from 7.2.8 to 7.3.0 (aws-powertools#695)
  chore(deps-dev): bump mkdocs-material from 7.2.6 to 7.2.8 (aws-powertools#682)
  chore(deps-dev): bump flake8-bugbear from 21.4.3 to 21.9.1 (aws-powertools#676)
  chore(deps): bump boto3 from 1.18.38 to 1.18.41 (aws-powertools#677)
  chore(deps-dev): bump radon from 4.5.2 to 5.1.0 (aws-powertools#673)
  chore(deps): bump boto3 from 1.18.32 to 1.18.38 (aws-powertools#671)
  refactor(data-classes): clean up internal logic for APIGatewayAuthorizerResponse (aws-powertools#643)
  fix(data-classes): use correct asdict funciton (aws-powertools#666)
  chore(deps-dev): bump xenon from 0.7.3 to 0.8.0 (aws-powertools#669)
  chore: bump to 1.20.2
  fix: Fix issue with strip_prefixes (aws-powertools#647)
  chore(deps-dev): bump mkdocs-material from 7.2.4 to 7.2.6 (aws-powertools#665)
  chore(deps): bump boto3 from 1.18.26 to 1.18.32 (aws-powertools#663)
  chore(deps-dev): bump pytest from 6.2.4 to 6.2.5 (aws-powertools#662)
  chore(license): Add THIRD-PARTY-LICENSES (aws-powertools#641)
@BVMiko BVMiko deleted the strip_prefixes-fix branch October 1, 2021 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants